Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type GUIDs to partitions resolver #2745

Merged
merged 3 commits into from
Aug 27, 2024
Merged

Conversation

mhashizume
Copy link
Contributor

@mhashizume mhashizume commented Aug 5, 2024

This PR adds partition type GUIDs to the partitions resolver.

In order to add this information, this PR also changes the partitions resolver to prefer lsblk over blkid. Previously, Facter would use blkid and fall back to lsblk only if blkid did not return anything, for whatever reason. However, there are several reasons to prefer lsblk:

  • lsblk can return more information (like partition type GUIDs) than blkid.
  • lsblk does not have to necessarily run as root, blkid does.
  • Newer versions of blkid's man page specifically recommend using lsblk:

It is recommended to use lsblk(8) command to get information about block devices, or lsblk --fs to get an overview of filesystems, or findmnt(8) to search in already mounted filesystems.

lsblk(8) provides more information, better control on output formatting, easy to use in scripts and it does not require root permissions to get actual information. blkid reads information directly from devices and for non-root users it returns cached unverified information. blkid is mostly designed for system services and to test libblkid(3) functionality.

@mhashizume mhashizume added the enhancement New feature or enhancement label Aug 5, 2024
@mhashizume
Copy link
Contributor Author

I still need to add tests and make sure that lsblk returns information in the same order as blkid for a stable API.

return {} if lsblk_version < 2.23

# lsblk 2.25 added support for GPT partition type GUIDs
blkid_and_lsblk[:lsblk] ||= if lsblk_version >= 2.25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might it be easier to run lsblk with the longer args and rescue if it errors out?

@mhashizume mhashizume force-pushed the gpt-guids branch 3 times, most recently from b2abca0 to 45ee919 Compare August 22, 2024 23:16
@mhashizume mhashizume marked this pull request as ready for review August 23, 2024 17:18
@mhashizume mhashizume requested a review from a team as a code owner August 23, 2024 17:18
@mhashizume
Copy link
Contributor Author

Closing and reopening to grab latest tests.

@mhashizume mhashizume closed this Aug 23, 2024
@mhashizume mhashizume reopened this Aug 23, 2024
mhashizume and others added 3 commits August 23, 2024 10:46
Ruby 2.4 introduced the compact method to drop nil values from a Hash.
Since Facter recommends Ruby 2.5 and above, we are able to replace more
lengthy logic with Hash#compact instead.
Prior to this commit, the partitions resolver only used lsblk as a
fallback if blkid did not return any information.

However, there are many advantages to lsblk. lsblk does not necessarily
require root (while blkid does), it returns more information (like
partition type GUIDs), and blkid specifically recommends using lsblk in
its manpage.

This commit updates the partitions resolver to prefer lsblk and use
blkid as a fallback.
This commit adds a new fact to the partitions fact hash, partition type
GUID.

Co-authored-by: Michael Hashizume <[email protected]>
@mhashizume
Copy link
Contributor Author

I've updated the branch protection rule to no longer require integration tests against JRuby 9.3.90 or 9.4.7.0 (these tests were updated in #2740)

@cthorn42 cthorn42 merged commit b6cf607 into puppetlabs:main Aug 27, 2024
18 checks passed
@mhashizume mhashizume deleted the gpt-guids branch October 15, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants